docs(config): add descriptions to all configuration variables and tests #682

onyedikachi-david · 2024-10-11T15:15:37Z

Closes #40

/claim #40

Changes proposed

Added descriptions to all configuration variables in OpalClientConfig
Added descriptions to all configuration variables in OpalServerConfig
Added descriptions to all configuration variables in OpalCommonConfig
Created a test to ensure all configuration variables have descriptions

Check List (Check all the applicable boxes)

I sign off on contributing this submission to open-source
My code follows the code style of this project.
My change requires changes to the documentation.
I have updated the documentation accordingly.
All new and existing tests passed.
This PR does not contain plagiarised content.
The title of my pull request is a short description of the requested changes.

Signed-off-by: David Anyatonwu <[email protected]>

netlify · 2024-10-11T15:15:55Z

✅ Deploy Preview for opal-docs canceled.

Name	Link
🔨 Latest commit	`f38be07`
🔍 Latest deploy log	https://app.netlify.com/sites/opal-docs/deploys/670cb6503eea510008cf6b54

onyedikachi-david · 2024-10-12T12:54:58Z

The PR is ready for review @gemanor

gemanor

It is a good start, but the descriptions need to be much more descriptive. I left two examples (but there are more). Please fix them, and commit again.

gemanor · 2024-10-13T20:58:49Z

packages/opal-client/opal_client/config.py

+    POLICY_STORE_URL = confi.str(
+        "POLICY_STORE_URL",
+        "http://localhost:8181",
+        description="URL of the policy store"


This is exactly what written on the key, please explain more...

gemanor · 2024-10-13T21:00:24Z

packages/opal-client/opal_client/config.py

+    INLINE_OPA_ENABLED = confi.bool(
+        "INLINE_OPA_ENABLED",
+        True,
+        description="Whether to run OPA inline within OPAL"


Even the code comment has more description of this description. The task is to write descriptions of the variables, not just mark them as solved. Please do some understanding work around OPAL, and explain the configuration variables in a concise but descriptive way.

Signed-off-by: David Anyatonwu <[email protected]>

onyedikachi-david · 2024-10-14T06:13:11Z

It is a good start, but the descriptions need to be much more descriptive. I left two examples (but there are more). Please fix them, and commit again.

Done.

onyedikachi-david · 2024-10-25T12:24:33Z

@gemanor Please review.

onyedikachi-david · 2024-11-17T15:39:31Z

Hi @gemanor, I don't know if you have got time to review this.

onyedikachi-david · 2024-11-17T15:39:52Z

Hi @gemanor, I don't know if you have got time to review this.

iwphonedo

@onyedikachi-david nice job. Please see my comments inside. Almost there. good luck and don't hesitate to contact me for any help needed.

iwphonedo · 2024-12-24T16:17:29Z

packages/opal-client/opal_client/config.py

+        "POLICY_STORE_TYPE",
+        PolicyStoreTypes,
+        PolicyStoreTypes.OPA,
+        description="Specifies the type of policy store to use. Currently supports OPA (Open Policy Agent) and Cedar. This determines how OPAL interacts with the policy engine."


@onyedikachi-david I would add how it affects the interaction.

iwphonedo · 2024-12-24T16:20:57Z

packages/opal-client/opal_client/config.py

@@ -65,14 +74,14 @@ class OpalClientConfig(Confi):
            "attempts": 5,
            "wait_time": 1,
        },
-        description="retry options when connecting to the policy source (e.g. the policy bundle server)",
+        description="Retry options when connecting to the policy source (e.g., the policy bundle server). Configures how OPAL handles connection failures when fetching policy updates."


@onyedikachi-david wait time in seconds

iwphonedo · 2024-12-24T16:37:27Z

packages/opal-client/opal_client/config.py

+    INLINE_CEDAR_ENABLED = confi.bool(
+        "INLINE_CEDAR_ENABLED",
+        True,
+        description="Determines whether OPAL should run the Cedar agent inline within its own container. Similar to INLINE_OPA_ENABLED, but for Cedar policy engine."


@onyedikachi-david please add if both options could be enabled at the same time opa and cedar

iwphonedo · 2024-12-24T17:04:36Z

packages/opal-client/opal_client/config.py

    )

    OPAL_CLIENT_STAT_ID = confi.str(
        "OPAL_CLIENT_STAT_ID",
        None,
-        description="Unique client statistics identifier",
+        description="Unique identifier for client statistics. This can be used to track and differentiate between multiple OPAL clients in a distributed setup."
    )

    OPA_HEALTH_CHECK_POLICY_PATH = "engine/healthcheck/opal.rego"


@onyedikachi-david missing here

iwphonedo · 2024-12-24T17:12:31Z

packages/opal-common/opal_common/config.py

@@ -5,18 +5,24 @@
 from opal_common.confi import Confi, confi

 _LOG_FORMAT_WITHOUT_PID = "<green>{time}</green> | <blue>{name: <40}</blue>|<level>{level:^6} | {message}</level>\n{exception}"
-_LOG_FORMAT_WITH_PID = "<green>{time}</green> | {process} | <blue>{name: <40}</blue>|<level>{level:^6} | {message}</level>\n{exception}"
+_LOG_FORMAT_WITH_PID = "<green>{time}</green> | {process} | <blue>{name: <40></blue>|<level>{level:^6} | {message}</level>\n{exception}"


@onyedikachi-david why > and not }?

iwphonedo · 2024-12-24T17:56:18Z

packages/opal-server/opal_server/config.py

    POLICY_REPO_MANIFEST_PATH = confi.str(
        "POLICY_REPO_MANIFEST_PATH",
        "",
-        "Path of the directory holding the '.manifest' file (new fashion), or of the manifest file itself (old fashion). Repo's root is used by default",
+        "Path to the directory containing the '.manifest' file, or to the manifest file itself. "
+        "The manifest file is used to specify which files in the repo should be treated as policies.",


@onyedikachi-david where is the spec about that file, and its content format?

iwphonedo · 2024-12-24T17:57:21Z

packages/opal-server/opal_server/config.py

-        "LEADER_LOCK_FILE_PATH", "/tmp/opal_server_leader.lock"
+        "LEADER_LOCK_FILE_PATH",
+        "/tmp/opal_server_leader.lock",
+        description="Path to the leader lock file. Used in multi-server setups to determine the primary server."


@onyedikachi-david multi server might be a bit confusing. I think this is for the number of uviworkers in the server, to set the process that is the leader. am I right?

iwphonedo · 2024-12-24T18:13:45Z

packages/opal-server/opal_server/config.py

    )
    POLICY_BUNDLE_GIT_ADD_PATTERN = confi.str(
        "POLICY_BUNDLE_GIT_ADD_PATTERN",
        "*",
-        description="File pattern to add files to git default to all the files (*)",
+        description="File pattern to add files to git. Default is all files (*).",


@onyedikachi-david not clear what is this? which files are added to git?

iwphonedo · 2024-12-24T18:29:23Z

packages/opal-server/opal_server/config.py


    SCOPES_REPO_CLONES_SHARDS = confi.int(
        "SCOPES_REPO_CLONES_SHARDS",
        1,
-        description="The max number of local clones to use for the same repo (reused across scopes)",
+        description="The maximum number of local clones to use for the same repo (reused across scopes). This helps in managing multiple scopes efficiently."


@onyedikachi-david this one needs more explanation. what are those shards and their relationship to the scopes

iwphonedo · 2024-12-24T18:30:37Z

packages/opal-server/opal_server/config.py


+    # Policy refresh configuration
    POLICY_REFRESH_INTERVAL = confi.int(


@onyedikachi-david isn't this POLICY_REPO_POLLING_INTERVAL? What is the difference between the two?

onyedikachi-david · 2024-12-24T18:40:15Z

@iwphonedo, I thought it's closed. Even though I open a PR first and my re-review request was pending when the other PR was opened. Should I reopen?

iwphonedo · 2024-12-25T19:31:46Z

@onyedikachi-david send like another PR was already merged for this. Sorry I didn't notice prior to my review. Best

onyedikachi-david · 2024-12-27T15:16:54Z

Okay

onyedikachi-david added 2 commits October 11, 2024 15:54

docs(config): add descriptions to all configuration variables and tests

b576f4f

Signed-off-by: David Anyatonwu <[email protected]>

chore: added missing descriptions

f65752b

Signed-off-by: David Anyatonwu <[email protected]>

algora-pbc bot mentioned this pull request Oct 11, 2024

Go over all OPAL's config variables and add descriptions #40

Closed

algora-pbc bot added the 🙋 Bounty claim label Oct 11, 2024

gemanor requested changes Oct 13, 2024

View reviewed changes

chore: added detailed config descriptions

f38be07

Signed-off-by: David Anyatonwu <[email protected]>

danyi1212 closed this Dec 24, 2024

iwphonedo suggested changes Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(config): add descriptions to all configuration variables and tests #682

docs(config): add descriptions to all configuration variables and tests #682

onyedikachi-david commented Oct 11, 2024 •

edited

Loading

netlify bot commented Oct 11, 2024 •

edited

Loading

onyedikachi-david commented Oct 12, 2024

gemanor left a comment

gemanor Oct 13, 2024

gemanor Oct 13, 2024

onyedikachi-david commented Oct 14, 2024

onyedikachi-david commented Oct 25, 2024

onyedikachi-david commented Nov 17, 2024

onyedikachi-david commented Nov 17, 2024

iwphonedo left a comment

iwphonedo Dec 24, 2024

iwphonedo Dec 24, 2024

iwphonedo Dec 24, 2024

iwphonedo Dec 24, 2024

iwphonedo Dec 24, 2024

iwphonedo Dec 24, 2024

iwphonedo Dec 24, 2024

iwphonedo Dec 24, 2024

iwphonedo Dec 24, 2024

iwphonedo Dec 24, 2024

onyedikachi-david commented Dec 24, 2024

iwphonedo commented Dec 25, 2024

onyedikachi-david commented Dec 27, 2024


		# Policy refresh configuration
		POLICY_REFRESH_INTERVAL = confi.int(

docs(config): add descriptions to all configuration variables and tests #682

docs(config): add descriptions to all configuration variables and tests #682

Conversation

onyedikachi-david commented Oct 11, 2024 • edited Loading

Closes #40

Changes proposed

Check List (Check all the applicable boxes)

netlify bot commented Oct 11, 2024 • edited Loading

✅ Deploy Preview for opal-docs canceled.

onyedikachi-david commented Oct 12, 2024

gemanor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onyedikachi-david commented Oct 14, 2024

onyedikachi-david commented Oct 25, 2024

onyedikachi-david commented Nov 17, 2024

onyedikachi-david commented Nov 17, 2024

iwphonedo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onyedikachi-david commented Dec 24, 2024

iwphonedo commented Dec 25, 2024

onyedikachi-david commented Dec 27, 2024

onyedikachi-david commented Oct 11, 2024 •

edited

Loading

netlify bot commented Oct 11, 2024 •

edited

Loading